Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Let filt/filt! return state if provided as input #599

Closed
wants to merge 1 commit into from

Conversation

martinholters
Copy link
Member

As argued in #372 (comment), I don't think the possibility to pass in state to filt and filt! in the current form is very helpful, as there is no supported way of obtaining such state. There is the internal _zerosi, but explicitly passing the zero-state is equivalent to passing no state, so why bother. Otherwise, the filter state should probably be considered an implementation detail and it is undocumented, so hand-crafting it is not recommended. (With the exception of the use in DSPs own filtfilt, maybe.) The most likely use of passing in state is to resume a filtering operation, e.g. for blockwise processing. (Yes, that's what using a DF2TFilter allows, too.) But to enable that, one would need the final state to be provided by filt/filt!.

And that's what this PR does, but only if an input state is provided. I.e. one can do

(outblock1, state) = filt(H, inblock1, state)
(outblock2, state) = filt(H, inblock2, state)

The implementation is somewhat rough-and-dirty for now, as I first wanted to get a feel for this and see if we want to do this. There are some questions on the API worth thinking about:

  • How should the initial state be obtained? Make _zerosi public (with a better name)? Or support some placeholder value for the state parameter that indicates a zero-state should be set up?
  • Should filt! modify the state instead of returning it? Seems reasonable, but up to now, care was taken not to modify the state.
  • For multi-column input, we support passing in a single state which will then be replicated for each column. Do we need to keep supporting that?

Putting on the milestone as this would be breaking.

@martinholters martinholters added this to the 0.8 milestone Dec 2, 2024
Copy link

codecov bot commented Dec 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.92%. Comparing base (710ee46) to head (6d5e1e5).
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #599   +/-   ##
=======================================
  Coverage   97.91%   97.92%           
=======================================
  Files          19       19           
  Lines        3262     3272   +10     
=======================================
+ Hits         3194     3204   +10     
  Misses         68       68           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wheeheee
Copy link
Member

wheeheee commented Dec 3, 2024

Would it be too radical to remove si here and let those who care about state use DF2TFilter? Otherwise:

  • I think something like a public zerosi would be better since it might help users get some idea of the dimensions of the state, although dispatching on say ::ZeroSI does strike me as more aesthetic.
  • The change to returning a tuple with the state is sufficiently breaking, so preserving backwards-compatibility for not mutating state here isn't so important imo, but see below
  • I feel passing in a single state for multiple columns is convenient and should be kept. But if so, the state passed in has different dimensions and it doesn't make sense to mutate it, hence it might be inconsistent if we only mutate si in the case of multidimensional input + state...

@martinholters
Copy link
Member Author

martinholters commented Dec 3, 2024

Would it be too radical to remove si here and let those who care about state use DF2TFilter?

I've been wondering the same. I'll give it try and open an alternative PR if it looks promising. EDIT: PR up at #603.

  • I think something like a public zerosi would be better since it might help users get some idea of the dimensions of the state, although dispatching on say ::ZeroSI does strike me as more aesthetic.

Another point in favor of a zerosi would be a use-case where you do block-wise processing in a for-loop. It would be inconvenient (or type-unstable) to have to special-case the first-block to feed in a place-holder state. Better to allocate the state using a dedicated function before the loop.

  • The change to returning a tuple with the state is sufficiently breaking, so preserving backwards-compatibility for not mutating state here isn't so important imo, but see below

Right. That would allow the return-type to remain the same, but then this might be silently breaking, i.e. the same code still works, but produces different results. We might want to avoid that. Also, filt should probably still not mutate the state, which asks for yet another entry-point which mutates the state, but contrary to filt! allocates the output. This sounds less attractive the more I think about it, maybe getting rid of si in favor of DF2TFilter really is the way to go.

  • I feel passing in a single state for multiple columns is convenient and should be kept. But if so, the state passed in has different dimensions and it doesn't make sense to mutate it, hence it might be inconsistent if we only mutate si in the case of multidimensional input + state...

We could make that orthogonal by introducing some state-setup function that could both play the role of zerosi or do the appropriate repeat dance to fan out a single input state for multiple signals.

@martinholters
Copy link
Member Author

Superseded by #603.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants